Skip to content

fix(Makefile): prevent NUM_VALIDATORS from evaluating to two tokens - #339

Open
Ocheretovich wants to merge 1 commit into
circlefin:mainfrom
Ocheretovich:patch-1
Open

fix(Makefile): prevent NUM_VALIDATORS from evaluating to two tokens#339
Ocheretovich wants to merge 1 commit into
circlefin:mainfrom
Ocheretovich:patch-1

Conversation

@Ocheretovich

@Ocheretovich Ocheretovich commented Sep 4, 2026

Copy link
Copy Markdown

Fixed a bug in NUM_VALIDATORS: when the manifest had no [nodes.validator...] entries, grep -c returned 0 but exited with status 1, which triggered the || echo 5 fallback resulting in the variable evaluating to "0 5" instead of a single number. The check is now explicit: fall back to 5 only when grep's count is missing or zero, otherwise use the actual count.

I tested the old and new logic against three cases missing manifest, manifest with zero validators, and manifest with 3 validators. The first and third cases behave identically either way, but the middle one exposes the bug: the old logic returned "0\n5" (two values instead of one) when the manifest existed but had no validator entries, while the fix correctly returns 5.

@osr21

osr21 commented Sep 11, 2026

Copy link
Copy Markdown

Disclosure: I’m an external community contributor, not affiliated with Circle, not a maintainer, and I have no write access to this repository.

This is advisory review only.\n\nI independently re-ran the old and new Makefile expressions against the three relevant manifest states, plus a missing-file control:\n\n| manifest | old expression | this PR |\n|---|---:|---:|\n| missing file | 5 | 5 |\n| existing file with no validator sections | 0 5 | 5 |\n| existing file with three validator sections | 3 | 3 |\n\nThe reported bug is real, and the patch fixes the actual failure without changing the existing fallback policy. The important detail is that GNU grep -c prints 0 but exits with status 1 when there are no matches; the old || echo 5 therefore appended a second value.

The revised shell conditional treats a positive count as authoritative and falls back only for an empty/zero/non-numeric result.\n\nThe shell syntax is POSIX-compatible (make invokes /bin/sh here), and QUAKE_MANIFEST remains recursively expanded, so targets that override the manifest still recompute NUM_VALIDATORS as intended. I also checked the current Makefile usage: the value is passed as the single --num-validators argument to the genesis command, so eliminating the second token directly removes the malformed invocation.\n\nI did not find a correctness issue in the three-line change. The three cases in the PR description are the right regression matrix; the missing-file case is a useful control because it confirms the fallback behavior was not accidentally changed. This looks ready from the code/behavior side, subject to the repository’s normal CI/maintainer review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants